Skip to content

Migrate TracerHealthMetrics onto the Accumulator primitive - #12383

Draft
dougqh wants to merge 16 commits into
dougqh/accumulator-primitivefrom
dougqh/accumulator-tracerhealthmetrics
Draft

Migrate TracerHealthMetrics onto the Accumulator primitive#12383
dougqh wants to merge 16 commits into
dougqh/accumulator-primitivefrom
dougqh/accumulator-tracerhealthmetrics

Conversation

@dougqh

@dougqh dougqh commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

What Does This Do

Trial migration of TracerHealthMetrics onto Accumulator<E> (#12351), the concrete case cited in that PR's review discussion as the motivating ceremony.

  • Replaces ~49 LongAdder fields and the hand-rolled previousCounts[]/countIndex delta-tracking (with its ArrayIndexOutOfBoundsException safety-net catch) in Flush.run() with a single Accumulator<Metric> (nested TracerHealthMetrics.Metric enum), using accumulateAndReset() for the periodic drain.
  • summary() reads a live value (storedTotal.plus(counters.sum())) that never races the periodic Flush drain, using the primitive's non-destructive sum()/Counts.plus().
  • Adds reusable StatsDCounterKey/StatsDCountReporter glue in metrics-api, decoupled from internal-api via a ToLongFunction<E> accessor.
  • HealthMetricsTest and MetricsReliabilityTest pass unmodified — same statsd.count(...) call shape per flush, same summary() labels.

Motivation

Accumulator (#12351) had no real caller yet, and review pushed back on whether the abstraction earns its keep versus the status quo (LongAdder + hand-rolled delta tracking). Rather than keep arguing in the abstract, this wires it into TracerHealthMetrics — the concrete case already cited in that discussion — to settle the question on working code:

  • Ergonomics: eliminates the previousCounts/countIndex hand-tracking ceremony (Accumulator.accumulateAndReset() already returns the delta since the last drain) and the 49 individual reportIfChanged call sites, for a 187-line (28%) reduction in TracerHealthMetrics (656 → 469 lines vs. pre-migration at 77964b3996), despite now inlining all 54 Metric constants that previously lived in a separate file.
  • Performance: see benchmark results below — no longer a speed-for-ergonomics trade.

Additional Notes

Benchmark results (TracerHealthMetricsBenchmark, direct measurement of real call sites)

Accumulator was rewritten after this trial started to a lock-free AtomicLongArray-striped design (see #12351). Under that design, the new implementation is now at parity with, or measurably faster than, the legacy LongAdder baseline on every single-call-site benchmark, including under Threads.MAX contention. Confirmed stable across JDK 17 and JDK 25 (point estimates agree almost exactly between JVMs).

Apple M1 Max, 10 CPUs - macOS/aarch64 - JDK 17 (Zulu) / JDK 25 (Zulu)
Benchmark                       New (JDK17/25)  Legacy (JDK17/25)  Ratio
onCreateSpan_lowContention        0.007 / 0.007    0.007 / 0.007    1.0x / 1.0x
onCreateSpan_highContention       0.009 / 0.009    0.010 / 0.010    0.9x / 0.9x
onFailedPublish_lowContention     0.007 / 0.007    0.008 / 0.008    0.9x / 0.9x
onFailedPublish_highContention    0.010 / 0.010    0.011 / 0.012    0.9x / 0.8x
onPartialPublish_lowContention    0.007 / 0.007    0.008 / 0.008    0.9x / 0.9x
onPartialPublish_highContention   0.009 / 0.010    0.012 / 0.012    0.75x / 0.8x
onSend_lowContention              0.009 / 0.009    0.012 / 0.013    0.75x / 0.7x
onSend_highContention             0.013 / 0.013    0.020 / 0.022    0.65x / 0.6x
summaryWhileWriting_write         0.008 / 0.008    0.009 / 0.008    0.9x / 1.0x
summaryWhileWriting_read          1.835 / 1.833    0.705 / 0.720    2.6x / 2.55x
(all figures us/op, avgt; JDK17 / JDK25)

The only remaining cost is the diagnostic summary() read (walking all 54 stripes non-destructively, ~2.5-2.6x legacy) — well below the periodic 30s-default Flush cadence and the ad hoc/diagnostic calls that trigger it, so not disqualifying.

Note: AccumulatorBenchmark's own javadoc (on #12351) went through two corrections. A width-8, per-thread-distributed write-side comparison was added (the single-counter benchmarks were the degenerate worst case for the longAdderGroup baseline), and a Fork(5)/15-sample re-run showed the drain-side "regression" from an earlier correction (13.357 us/op, ~5.5x worse) was itself a correlated anomaly across two low-sample runs — the verified number is 2.746 us/op, roughly at parity with (and in this reading faster than) longAdderGroup's noisy 4.770 ± 1.795 us/op. Neither correction changes this PR's real-call-site numbers above, which were re-measured directly.

Test plan

  • ./gradlew :dd-trace-core:test --tests "datadog.trace.core.monitor.HealthMetricsTest" — 40/40 passing, no test-file changes
  • ./gradlew :dd-trace-core:test --tests "datadog.trace.common.metrics.MetricsReliabilityTest" — passing, no test-file changes
  • ./gradlew :products:metrics:metrics-api:test --tests "datadog.metrics.api.statsd.StatsDCountReporterTest" — new tests passing
  • ./gradlew :internal-api:test --tests "datadog.trace.util.AccumulatorTest" — passing (on base branch)
  • ./gradlew :dd-trace-core:jmh -Pjmh.includes=TracerHealthMetricsBenchmark — run on both JDK 17 and JDK 25, results above
  • /techdebt — clean, no fixable debt (this branch is itself a debt-removal commit)
  • /perf-review — 1 flag-as-measure finding (SEV-3, non-blocking, now stale — predates the update() removal, no capturing-lambda call sites remain on this branch)

Contributor Checklist

Jira ticket: APMLP-1779

🤖 Generated with Claude Code

@dougqh dougqh added comp: metrics Metrics type: refactoring tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes labels Sep 2, 2026
@datadog-datadog-prod-us1

This comment has been minimized.

@dd-octo-sts

dd-octo-sts Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 14.79 s 14.68 s [-0.0%; +1.5%] (no difference)
startup:insecure-bank:tracing:Agent 13.64 s 13.67 s [-1.2%; +0.8%] (no difference)
startup:petclinic:appsec:Agent 17.49 s 17.16 s [+0.9%; +3.0%] (maybe worse)
startup:petclinic:iast:Agent 17.48 s 17.50 s [-0.8%; +0.6%] (no difference)
startup:petclinic:profiling:Agent 17.57 s 17.33 s [+0.1%; +2.6%] (maybe worse)
startup:petclinic:sca:Agent 17.34 s 17.35 s [-0.9%; +0.9%] (no difference)
startup:petclinic:tracing:Agent 16.54 s 16.64 s [-1.7%; +0.4%] (no difference)

Commit: b2f791fa · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.


private final LongAdder statsAggregateDropped = new LongAdder();
private final LongAdder statsInboxFull = new LongAdder();
private final Accumulator<TracerHealthMetric> counters =

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this metricAccumulator instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to metricAccumulator.

private final LongAdder statsInboxFull = new LongAdder();
private final Accumulator<TracerHealthMetric> counters =
Accumulator.of(TracerHealthMetric.values());
private volatile Accumulator.Counts<TracerHealthMetric> storedTotal = counters.sum();

@dougqh dougqh Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should provide an API for creating a Counts directly. This feels a little awkward to me.

I'm also wondering if Counts should handle thread-safety, but I'm torn on that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Accumulator.Counts.zero(E[] values) (an all-zero Counts sized for the enum, no scratch Accumulator/sum() needed) and switched storedTotal's seeding to it. Left Counts itself as a plain immutable value type for now since you flagged you're torn on the thread-safety question -- happy to revisit if you land on an answer there.

switch (samplingPriority) {
case USER_DROP:
userDropEnqueuedTraces.increment();
enqueuedTracesMetric = TracerHealthMetric.USER_DROP_ENQUEUED_TRACES;

@dougqh dougqh Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the porting simple, I think let's just do inline increments here. I know that means taking the lock multiple times, but so be it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done -- onPublish now increments the priority-specific counter inline per switch case and adds ENQUEUED_SPANS separately, no update() lambda.

case USER_DROP:
userDropDroppedSpans.add(spanCount);
userDropDroppedTraces.increment();
droppedSpansMetric = TracerHealthMetric.USER_DROP_DROPPED_SPANS;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer either just do inline increments. Or if we can still coarsen and use, accumulator.update, but do inline in each switch case.

Right now, we're creating a capturing lambda which is something that we want to avoid.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same treatment here -- onFailedPublish now does inline per-case add/inc pairs instead of building droppedSpansMetric/droppedTracesMetric locals and closing over them in a single update() lambda.

if (trace != null) {
serialFailedDroppedTraces.increment();
serialFailedDroppedSpans.add(trace.size());
counters.update(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this case is slightly annoying.
We end up capturing the trace.
I guess we can a contextual variation on update that uses BiConsumer<C, Stripe> to solve that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added exactly the overload you sketched: Accumulator.update(C context, BiConsumer<C, Stripe<E>> mutator) on dougqh/accumulator-primitive (so it's shared with anyone else pulling in the primitive). Wired it into onFailedSerialize (passes trace.size() instead of capturing trace) and, for consistency, onPartialPublish too (passes numberOfDroppedSpans). Javadoc on the new overload flags the one caveat: a primitive context gets boxed, so it's a real allocation trade against the capturing lambda it replaces rather than a free win -- worth it here since it kills the capture, but not a blanket recommendation.

@dougqh
dougqh force-pushed the dougqh/accumulator-tracerhealthmetrics branch 2 times, most recently from 47365ce to 95b9180 Compare September 2, 2026 14:36
public final class StatsDCountReporter {
private StatsDCountReporter() {}

public static <E extends Enum<E> & StatsDCounterKey> void report(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering it a better option is take a Counts directly.
I'm also pondering whether Accumulator or Counts should provide a way to get the corresponding enum elements to reduce the amount of ceremony.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done both ways: Accumulator/Counts now remember the enum's values() array from construction, so Counts.values() lets a caller iterate its own keys -- StatsDCountReporter.report(StatsDClient, Counts<E>) uses that to collapse to one arg instead of TracerHealthMetric.values() + delta::get. Also added Accumulator.of(Class<E>)/Counts.zero(Class<E>) so metricAccumulator's construction reads as Accumulator.of(TracerHealthMetric.class). Landed on dougqh/accumulator-primitive (extra field on Counts, ~free since it's already a per-drain allocation) and wired in here.

@dougqh
dougqh force-pushed the dougqh/accumulator-tracerhealthmetrics branch 2 times, most recently from 6599253 to df7b5d4 Compare September 2, 2026 15:27
public void onPartialPublish(final int numberOfDroppedSpans) {
partialTraces.increment();
samplerDropDroppedSpans.add(numberOfDroppedSpans);
metricAccumulator.update(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a way to pass primitives as context without boxing. Or as least int or long?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Accumulator.update(long context, ObjLongConsumer<Stripe<E>> mutator) on #12351 — reuses the JDK's ObjLongConsumer instead of the generic BiConsumer, so the context is passed as a primitive long, no boxing. An int argument widens into it for free (also no boxing), so it covers both. I didn't add a separate int overload alongside it: passing a plain int literal/variable turned out ambiguous between the two (exact match for one, free widening to the other, and the differing functional-interface types block the usual most-specific tiebreak) — long alone is the simplest fix and costs nothing for the int case.

Updated onPartialPublish (and onFailedSerialize, same shape) to use it — just a lambda-arg-order swap since ObjLongConsumer#accept is (T, long) rather than (long, T).

statsd.incrementCounter("api.responses.total", statusTagsCache.get(status));
}
final int status = response.status().orElse(0);
metricAccumulator.update(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, that we can pass response as context we should use that.
I think we can divide into several updates to avoid capture & boxing.

At least here, I think using update is more best effort to avoid lock acquisition overhead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restructured onSendAttempt: API_REQUESTS/FLUSHED_TRACES/FLUSHED_BYTES are now three top-level inc/add calls (independent counters, same pattern as onPublish/onFailedPublish), and response is now passed as update's context for the two response-derived checks (API_ERRORS/API_RESPONSES_OK) — since it's already a reference, that's zero boxing, and grouping just those two under one update keeps a single lock acquisition for the part where it actually helps (they both need response.status()/response.exception()). The lambda no longer captures traceCount/sizeInBytes/response together.

I kept the post-lock response.status().orElse(0) call for the non-200 statsd.incrementCounter path recomputed rather than threaded through — it's a cheap Optional unwrap, and threading it out would've meant carrying a second context value. Let me know if you'd rather see that differently.

@dougqh
dougqh force-pushed the dougqh/accumulator-tracerhealthmetrics branch from df7b5d4 to daa0ae5 Compare September 2, 2026 16:28
dougqh and others added 11 commits September 2, 2026 14:15
Replaces ~49 LongAdder fields plus the hand-rolled previousCounts[]
delta-tracking in Flush.run() with a single Accumulator<TracerHealthMetric>,
using accumulateAndReset() for the periodic drain and sum()/plus() to give
summary() a live total that never races that drain. Adds the reusable
StatsDCounterKey/StatsDCountReporter glue in metrics-api along the way.

HealthMetricsTest and MetricsReliabilityTest pass unmodified.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
….zero(), drop capturing lambdas from onPublish/onFailedPublish

- counters -> metricAccumulator for clarity.
- storedTotal is now seeded via the new Accumulator.Counts.zero(), instead
  of constructing a fresh Accumulator just to call sum() on it.
- onPublish/onFailedPublish now increment inline per switch case instead of
  building metric-selecting locals and combining them in a single
  capturing accumulator.update() lambda -- takes the per-stripe lock twice
  instead of once, in exchange for no lambda capture on these paths.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…cals

onFailedSerialize and onPartialPublish now pass their int context (trace
size, dropped-span count) as an explicit parameter to Accumulator.update()
instead of closing over a local, per review.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ethod ref

StatsDCountReporter.report(StatsDClient, Counts<E>) uses the primitive's
new Counts.values() to reduce this call site to one argument instead of
TracerHealthMetric.values() + delta::get. Also switch metricAccumulator's
construction to Accumulator.of(TracerHealthMetric.class)/Counts.zero(class)
now that those overloads exist.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Drops the shared Tags nested class in favor of literal string tags at
each enum constant, per the same reasoning as the earlier NO_TAGS
removal: the array-sharing only saved a one-time classload allocation.
…ed hot path

Measures the real onCreateSpan/onFailedPublish/onPartialPublish/onSend
call sites (not a synthetic stand-in) plus summary()'s peek-under-
concurrent-writers cost, checking AccumulatorBenchmark's raw-primitive
numbers against actual production call shapes.
LegacyTracerHealthMetrics faithfully reconstructs pre-migration
TracerHealthMetrics (as of 77964b3) so the new Accumulator-backed
implementation can be benchmarked against it in the same run/JVM, not
just argued about. Accumulator's per-call cost is 1.1-4.6x the LongAdder
baseline depending on contention and counter grouping, and summary()
costs ~4x more to walk; the migration's case rests on eliminating the
previousCounts/countIndex ceremony and atomic multi-field updates, not
on raw per-call speed.
@dougqh
dougqh force-pushed the dougqh/accumulator-tracerhealthmetrics branch from 7157bdd to 157d441 Compare September 2, 2026 18:16
dougqh and others added 2 commits September 2, 2026 17:14
…rray striping

Benchmarking showed the AtomicLongArray design beats the realistic
LongAdder-per-counter baseline by ~2 orders of magnitude on increment
(the hot path) at a small cost on drain (the rare, periodic path).
Per-counter atomicity is preserved via getAndSet/getAndAdd; row-wide
atomicity across counters is dropped since no real caller needs it,
so EmbeddingSupport, Stripe, and the update() overloads are removed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…erHealthMetric as Metric

The grouped update() call sites collapse to sequential inc/add calls
now that Accumulator no longer offers row-wide atomicity -- none of
the 6 sites relied on it as a correctness invariant. Also fold the
standalone TracerHealthMetric enum into a nested TracerHealthMetrics.Metric
to shorten both the type name and the file count.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* combination -- several constants can share a metric name but differ by tag, mirroring the
* distinct {@code LongAdder} fields this enum replaces.
*/
enum TracerHealthMetric implements StatsDCounterKey {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to rename this Metric and make an inner class of TraceHealthMetrics.

dougqh and others added 2 commits September 2, 2026 18:52
The previous numbers (2.740 us/op) didn't match either measured JMH
run on disk (both agree on ~13.4 us/op) -- Accumulator's drain is
~5.5x worse than longAdderGroup at high contention, not "slightly
worse". The increment-side win is also corrected (~50x, not ~2 orders
of magnitude) though the conclusion there is unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ator

The before/after numbers predated the AtomicLongArray-striping rewrite
(3ea8479) and described a design that no longer exists -- the new
implementation is now at parity with or faster than legacy LongAdder
on every single-call-site benchmark, confirmed stable across JDK 17
and JDK 25.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* combination -- several constants can share a metric name but differ by tag, mirroring the
* distinct {@code LongAdder} fields this enum replaces.
*/
enum Metric implements StatsDCounterKey {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this to the top

* as a standalone class here (not resurrected via checkout) purely for a same-run, same-JVM
* before/after comparison; it is not wired into anything and should never be.
*/
class LegacyTracerHealthMetrics {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll probably remove this from the final PR, but for now, it shows the performance difference.

…n, correct javadoc numbers

Width-1 benchmarks pinned every thread to one shared longAdderGroup lock,
the degenerate worst case for that baseline. New *8_* benchmarks pin each
thread to one of 8 counters instead, giving longAdderGroup a fair shot at
distributed locking. Also replaces an earlier javadoc correction that was
itself wrong: the 13.357 us/op drain figure it introduced was a correlated
anomaly across two low-sample runs, not reproducible at Fork(5)/15 samples.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: metrics Metrics tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes type: refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant